Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Limit isort.lines-after-imports to 1 for stub files #9971

Merged
merged 1 commit into from
Feb 28, 2024

Conversation

MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Feb 13, 2024

Summary

Setting the isort.lines-after-imports setting to a value larger than 1 leads to formatter incompatibilities when formatting typing stub files because
the formatter enforces at most one blank line for stub files according to typeshed's style guide.

This PR changes our isort implementation to limit lines-after-imports to 1 for stub files the same as isort does for black (commit, code).

Fixes #9353

Breaking Change?

This is a breaking change and we may need to wait for 0.3 to merge. The only "excuse" not to consider this a breaking change is that we say our isort configuration is intended to be black/ruff formatter compatible by default (isort profile black). In this case, this is a bug fix.

I did a code search on GitHub for lines-after-imports=2 (I didn't find any usages with values > 2). Most repositories don't have pyi files and are unaffected by the change. The following repositories contain pyi files and use lines-after-imports=2

From that it seems safe to conclude that using lines-after-imports=2 with typing files isn't common, but at least two projects would see new errors after upgrading.

Test Plan

Added test

@MichaReiser MichaReiser added the isort Related to import sorting label Feb 13, 2024
@MichaReiser MichaReiser force-pushed the isort-lines-after-imports-in-stub-files branch from 7336890 to d00bb85 Compare February 13, 2024 08:34
Copy link
Contributor

github-actions bot commented Feb 13, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@MichaReiser MichaReiser force-pushed the isort-lines-after-imports-in-stub-files branch from d00bb85 to 91222f5 Compare February 13, 2024 09:02
@MichaReiser MichaReiser marked this pull request as ready for review February 13, 2024 09:02
@charliermarsh
Copy link
Member

To confirm my understanding: if a project today doesn't set lines-after-imports, we do use 1 as a default in .pyi files, right?

@MichaReiser
Copy link
Member Author

To confirm my understanding: if a project today doesn't set lines-after-imports, we do use 1 as a default in .pyi files, right?

Is this a very friendly hint that my implementation is wrong because it is wrong ;) It uses -1 which uses 2 empty lines if the following statement is a class or function. I have to add a test and fix the implementation.

@MichaReiser MichaReiser force-pushed the isort-lines-after-imports-in-stub-files branch from 91222f5 to 51d4857 Compare February 14, 2024 13:33
@MichaReiser
Copy link
Member Author

To confirm my understanding: if a project today doesn't set lines-after-imports, we do use 1 as a default in .pyi files, right?

I think I know understand why you asked this question... Yes, the change isn't just a breaking change when using lines-after-imports=2 but also when using the default configuration if the imports are directly followed by a class or function declaration (which I assume is fairly common).

I think that's reason enough for us to wait with merging until 0.3

@MichaReiser MichaReiser added the breaking Breaking API change label Feb 14, 2024
@MichaReiser MichaReiser added this to the v0.3.0 milestone Feb 14, 2024
@charliermarsh
Copy link
Member

I asked because my guess was that it wasn't a breaking change for files that don't set lines-after-imports, because I thought we already limited to 1 in stub files. But it sounds like my guess was wrong :D

@MichaReiser MichaReiser merged commit 1791e7d into main Feb 28, 2024
17 checks passed
@MichaReiser MichaReiser deleted the isort-lines-after-imports-in-stub-files branch February 28, 2024 16:36
nkxxll pushed a commit to nkxxll/ruff that referenced this pull request Mar 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking API change isort Related to import sorting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ruff format insist on "single line after imports" for pyi files
2 participants